Skip to content

connect, session: Spark Connect group 1 translator arms and sql_command (P3-T012a part 2b, PR-12; stacked on #772) - #774

Open
techgopal wants to merge 4 commits into
p3/s1-t012a-ir-v2from
p3/s1-t012a-arms
Open

techgopal wants to merge 4 commits into
p3/s1-t012a-ir-v2from
p3/s1-t012a-arms

Conversation

@techgopal

Copy link
Copy Markdown
Contributor

P3-T012a part 2b (PR-12): the Spark Connect translator arms for group 1. Stacked on #772 (part 2a, the IR); the base is that branch, so the diff is part 2b only. Merge #772 first.

  • local_relation and cached_local_relation (two-pass: the translator records requests, connect-server builds them with Session::local_relation, a second pass resolves them; no async trait). No artifact store on this base yet, so every cached hash answers NOT_FOUND_CACHED_LOCAL_RELATION; chunked_cached_local_relation stays 56K00.
  • Read.DataSource.schema, SubqueryAlias.qualifier, nested array/map/struct/specialized_array literals, expression_string (SQL parser, ParserOptions in TranslateConf), the is_local arm.
  • sql_command (what spark.sql() sends): Session::sql_command (unstable-ir) tells a command from a query; a command runs at once and answers a LocalRelation of its rows then a metrics response; a query answers the SQL relation to re-send. Both the PySpark 4.x input field and the deprecated sql field work.
  • Coverage table rows and generated docs/json updated (cargo xtask connect-coverage --check clean).

Opus diff review: ops/reviews/2026-11-15-sonnet-1-t012a-part2b-diff.md (blocker, 3 majors, 2 minors fixed; see its Fixes section).
Known gaps: a root SQL relation selecting from a temp view fails ("views not supported" in the plan analyzer on this base); parameterised SQL and WithRelations input refused; per-session byte budget for inline data is a follow-up. Golden-pending (Spark text/classes unverified) items are listed in the review and handoff.
Not run locally: the full session and exec integration suites; CI covers them.

local_relation and cached_local_relation (a host pre-pass builds the
relations: the translator records requests on a first pass, the server
builds them with Session::local_relation, a second pass resolves them),
Read.DataSource.schema, SubqueryAlias.qualifier, nested array/map/struct/
specialized_array literals, expression_string through krishiv-sql, and the
is_local arm. Coverage rows and PySpark goldens updated.

Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
A SqlCommand runs through the new Session::sql_command (unstable-ir): a
command (any statement but a query) runs at once and answers a LocalRelation
of its rows as sql_command_result; a query is only analyzed and answers the
SQL relation the client re-sends. translate_plan translates a sql_command as
that SQL relation (sql_command_relation); input and parameters are refused.
Coverage row sql_command is Partial.

Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
- sql_command: use SqlCommand.input (what PySpark 4.x sends) when it is a SQL
  relation, answering it unchanged; WithRelations stays unsupported, any
  other relation is SQL_COMMAND_EXPECTS_SQL_OR_WITH_RELATIONS.
- A command answers a metrics response after the result (as Spark does);
  the module doc is corrected.
- LocalRelations::take_requests drains instead of cloning; the first-pass
  state is dropped before the build loop.
- Nested-literal member mismatches are CONNECT_INVALID_PLAN.ASSERTION_FAILURE,
  not INTERNAL_ERROR.
- A command result is capped at 0.7 x maxBatchSize; its time zone is read
  after the command ran.
- Docs: alias_name error class, connect CLAUDE.md, coverage row.

Signed-off-by: gopal tathe <gopal@users.noreply.github.com>
Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
@coderabbitai

coderabbitai Bot commented Nov 15, 2026 •

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

🗂️ Base branches to auto review: 1

  • main

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • Trigger review
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

@chatgpt-codex-connector

Copy link
Copy Markdown
Codex Review Summary: ❌ Failed

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ❌ Failed 2026-11-15T03:29:20.688695Z
We couldn’t complete this review. Comment @codex review to try again.
c824aaf PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

@techgopal

Copy link
Copy Markdown
Contributor Author

Diff review passed (Opus, ops/reviews/2026-11-15-sonnet-1-t012a-part2b-diff.md): the blocker (SqlCommand.input), the 3 majors and both minors are fixed; remaining nits are listed in the review.

@codecov

codecov Bot commented Nov 15, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@codspeed

codspeed Bot commented Nov 15, 2026

Copy link
Copy Markdown

Merging this PR will degrade performance by 10.77%

2 warnings

⚠️ 28 benchmarks are dominated by system calls

System calls cannot be simulated consistently and are left out of the measure. The reported value understates the real cost. Switch to the Walltime instrument to measure them.

Learn more about system calls

See affected benchmarks

⚠️ 1 benchmark with a significant change ran on different hardware

Its base and head results were measured on machines with a different CPU, core count or memory, so the change might not come from the code. Run base and head on the same runner type.

Learn more about Macro Runners

See affected benchmarks

❌ 1 regressed benchmark
✅ 223 untouched benchmarks
⏩ 5 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Benchmark BASE HEAD Efficiency
❌ parquet_scan_512_row_groups 122.7 ms 137.5 ms -10.77%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing p3/s1-t012a-arms (9f51e25) with p3/s1-t012a-ir-v2 (c15e33a)

Open in CodSpeed

Footnotes

  1. 5 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

techgopal added a commit that referenced this pull request Nov 15, 2026
dtolnay/rust-toolchain's `stable` branch was rewritten on 2026-11-15
02:32 UTC. The pinned commit b725eda5 no longer has history in that
repository, so zizmor's impostor-commit audit now fails on every PR that
touches a workflow (#769, #774). Repin the four uses (setup-rust action,
binaries, docs-site) to the current `stable` head 39008827. Its
action.yml is byte-identical to the old pin's, so the steps behave the
same.

Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant